Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

resetBrowser #237

Merged
merged 4 commits into from
Jul 14, 2019
Merged

resetBrowser #237

merged 4 commits into from
Jul 14, 2019

Conversation

UziTech
Copy link
Contributor

@UziTech UziTech commented May 7, 2019

Summary

Add jestPuppeteer.resetBrowser() function to reset the global.browser object according to the configuration.

global.browser is is setup based on the configuration.

this.global.browser = await puppeteer.connect({
  ...config.connect,
  ...config.launch,
  browserURL: undefined,
  browserWSEndpoint: wsEndpoint,
})

This will allow the browser to be reset by the user and ensure the global.browser object will be the same after this call as the when it was first setup.

Users can reset the global.browser object before each test by calling jestPuppeteer.resetBrowser() in a beforeEach function easily.

beforeEach(async () => {
  await jestPuppeteer.resetBrowser()
})

fixes #183 (comment)

Test plan

describe('resetBrowser', () => {
  test('should reset browser', async () => {
    const oldBrowser = browser
    await jestPuppeteer.resetBrowser()
    expect(browser).not.toBe(oldBrowser)
    // eslint-disable-next-line no-underscore-dangle
    expect(browser._connection._closed).toBe(false)
    // eslint-disable-next-line no-underscore-dangle
    expect(oldBrowser._connection._closed).toBe(true)
  })
})

@UziTech UziTech mentioned this pull request May 7, 2019
Merged
@UziTech
Copy link
Contributor Author

UziTech commented May 7, 2019

I'm not sure how to get the browser.closed state without using browser._connection._closed in the tests. It would be better if there was a browser.isClosed() function like on page but I don't think there is anything like that.

@UziTech UziTech mentioned this pull request May 8, 2019
Merged
@xiaoyuhen
Copy link
Contributor

hi @UziTech

thanks for your great work! I will take a look before this Sunday.

aslushnikov pushed a commit to puppeteer/puppeteer that referenced this pull request May 9, 2019
@UziTech @aslushnikov
feat(browser): Add browser.isConnected() (#4403)
511dcf9 
Add `browser.isConnected()` to the public api to be able to tell when the browser is connected

fixes argos-ci/jest-puppeteer#237 (comment)
@UziTech
Copy link
Contributor Author

UziTech commented May 10, 2019

puppeteer/puppeteer#4403 did get merged so there will be a browser.isConnected() function in the next release of puppeteer.

Should we wait to merge this until that is released? Or we could just change the test later.

@gregberge
Copy link
Member

gregberge commented May 10, 2019 •
edited
Loading

Hmm, we support also old versions of Puppeteer, so we should not rely on this method.

@UziTech
Copy link
Contributor Author

UziTech commented May 10, 2019

That code is just in the test so we would just need to update puppeteer in devDependencies.

That code isn't crucial so I don't think it matters too much if we don't use .isConnected()

@gregberge
Copy link
Member

@UziTech if it is only in test, then it is OK for me, do you know when will it be released?

@UziTech
Copy link
Contributor Author

UziTech commented May 14, 2019

I updated puppeteer in devdependencies and use the new .isConnected() function in the test.

@gregberge
Copy link
Member

It looks good, @xiaoyuhen are you OK with it?

@UziTech
Copy link
Contributor Author

UziTech commented Jun 5, 2019

@xiaoyuhen @neoziro any progress on this?

@gregberge
Copy link
Member

@xiaoyuhen seems to be absent. I will try to take time to test it next week.

gregberge
gregberge reviewed Jun 17, 2019
View reviewed changes
README.md Outdated Show resolved Hide resolved
packages/jest-environment-puppeteer/README.md Outdated Show resolved Hide resolved
@UziTech
Copy link
Contributor Author

UziTech commented Jun 18, 2019

I fixed the spelling and rebased to fix the conflict.

@gregberge gregberge merged commit ae93739 into argos-ci:master Jul 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gregberge gregberge gregberge left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@UziTech @xiaoyuhen @gregberge